Marking s-s for now to be safe. This testcase does require the fuzzPriv extension to help with reliable reproduction (https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension) It may take a moment but this testcase does work reliably. I have tested with the latest m-c debug and ASan debug builds. STR: 1) put launcher.html and testcase.html in the same directory 2) launch Firefox with fuzzPriv extension and attached prefs.js 3) wait, this should take a few seconds but I've seen it take about a minute on ASan debug builds Assertion failure: IsIdle() || (mActivelyCollecting && mIncrementalPhase != GraphBuildingPhase) (Reentered CC during graph building), at /src/xpcom/base/nsCycleCollector.cpp:3840 #0 nsCycleCollector::FinishAnyCurrentCollection() /src/xpcom/base/nsCycleCollector.cpp:3838:3 #1 mozilla::CycleCollectedJSRuntime::OnGC(JSContext*, JSGCStatus) /src/xpcom/base/CycleCollectedJSRuntime.cpp:1468:7 #2 js::gc::GCRuntime::maybeCallBeginCallback() /src/js/src/jsgc.cpp:7245:5 #3 js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) /src/js/src/jsgc.cpp:7279:25 #4 js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) /src/js/src/jsgc.cpp:7475:25 #5 js::gc::GCRuntime::startGC(JSGCInvocationKind, JS::gcreason::Reason, long) /src/js/src/jsgc.cpp:7557:5 #6 js::gc::GCRuntime::gcIfRequested() /src/js/src/jsgc.cpp:7764:13 #7 js::gc::GCRuntime::gcIfNeededAtAllocation(JSContext*) /src/js/src/gc/Allocator.cpp:237:9 #8 bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) /src/js/src/gc/Allocator.cpp:192:14 #9 JSObject* js::Allocate<JSObject, (js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, js::Class const*) /src/js/src/gc/Allocator.cpp:52:17 #10 js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) /src/js/src/vm/NativeObject-inl.h:517:21 #11 NewObject(JSContext*, JS::Handle<js::ObjectGroup*>, js::gc::AllocKind, js::NewObjectKind, unsigned int) /src/js/src/jsobj.cpp:732:9 #12 js::NewObjectWithGivenTaggedProto(JSContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) /src/js/src/jsobj.cpp:793:26 #13 JS_NewObjectWithGivenProto(JSContext*, JSClass const*, JS::Handle<JSObject*>) /src/js/src/jsapi.cpp:2057:12 #14 mozilla::dom::BindingJSObjectCreator<mozilla::dom::Event>::CreateObject(JSContext*, JSClass const*, JS::Handle<JSObject*>, mozilla::dom::Event*, JS::MutableHandle<JSObject*>) /src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:2887:20 #15 mozilla::dom::EventBinding::Wrap(JSContext*, mozilla::dom::Event*, nsWrapperCache*, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>) /src/obj-firefox/dom/bindings/EventBinding.cpp:1401:11 #16 JSObject* mozilla::dom::EventBinding::Wrap<mozilla::dom::Event>(JSContext*, mozilla::dom::Event*, JS::Handle<JSObject*>) /src/obj-firefox/dist/include/mozilla/dom/EventBinding.h:105:12 #17 XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, xpcObjectHelper&, nsID const*, bool, nsresult*) /src/js/xpconnect/src/XPCConvert.cpp:758:23 #18 XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, nsresult*) /src/js/xpconnect/src/XPCConvert.cpp:345:16 #19 nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) /src/js/xpconnect/src/XPCWrappedJSClass.cpp:1266:22 #20 PrepareAndDispatch /src/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:120:28 #21 SharedStub (/home/user/workspace/browsers/m-c-1510138316-asan-debug/libxul.so+0x39eaec6) #22 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1118:51 ... see log.txt

Andrew it looks like you know this code, can you take a look? The full stack indicates SnowWhiteKiller is causing a media decoder shutdown, after some event dispatches js::GC ends up calling back into the cycle collector.

In the stack, JS is causing us to open a window, which spins the event loop, then that's going into sync XHR, which is also spinning the event loop, which causes us to destroy a HTMLVideoElement, which causes us to do MediaDecoder::Shutdown(), which dispatches an event, which runs JS, with allocates some memory, triggering the GC. I don't know see where exactly we are in the GC or whatever the first time, but maybe we're inside another sync XHR or something. Olli, do you know if we could do something better here, maybe with how the media shutdown happens? Thanks.

Hmm, right, the stack trace is missing the interesting bits.

I claim this is a MediaDecoder issue, or HTMLMediaElement::~HTMLMediaElement() or some such. Definitely stuff under HTMLMediaElement::~HTMLMediaElement() is such which shouldn't happen, no events should be dispatched there synchronously.

https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/dom/media/MediaDecoder.cpp#217-229 Will nsContentUtils::DispatchEventOnlyToChrome() dispatch events synchronously? Or is it bad to call nsContentUtils::DispatchEventOnlyToChrome() when CC is running? Assign this bug to Alastor who added the code.

DispatchEventOnlyToChrome dispatches DOM events synchronously yes, as DOM event dispatching by default is. It is possible that AsyncEventDispatcher could be used instead.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #9) > Assign this bug to Alastor who added the code. Bisection confirms that bug 1274919 is the culprit.

Assertion failure: IsIdle() || (mActivelyCollecting && mIncrementalPhase != GraphBuildingPhase) (Reentered CC during graph building), at /src/xpcom/base/nsCycleCollector.cpp:3840 This is just an assertion, right? Why is it classified as UAF?

That is a really bad assertion. Unknown things may happen if that assertion fires.

Comment on attachment 8930347 [details] [diff] [review] Bug 1416519 - use async event dispatcher. > void EnableEvent() const > { >- nsCOMPtr<nsPIDOMWindowOuter> win = GetOwnerWindow(); >- if (!win) { >+ nsIDocument* doc = GetOwnerDoc(); >+ if (!doc) { > return; > } >- nsContentUtils::DispatchEventOnlyToChrome( >- GetOwnerDoc(), ToSupports(win), Here you dispatch to window >- NS_LITERAL_STRING("UnselectedTabHover:Enable"), >- /* Bubbles */ true, >- /* Cancelable */ false, >- /* DefaultAction */ nullptr); >+ >+ RefPtr<AsyncEventDispatcher> asyncDispatcher = >+ new AsyncEventDispatcher(doc, but here to document. Looks like the listener doesn't care about the target

(In reply to Olli Pettay [:smaug] from comment #15) > Here you dispatch to window, > but here to document. Looks like the listener doesn't care about the target The listener is not a DOM object, so I can't get it as target directly. Since I don't familar with DOM event, I'm not sure the way we use to dispatch event is good or not. Here is the listener [1], is it good to send the event to document? Thanks! [1] https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/toolkit/content/browser-content.js#1130

(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #16) > (In reply to Olli Pettay [:smaug] from comment #15) > > Here you dispatch to window, > > but here to document. Looks like the listener doesn't care about the target > > The listener is not a DOM object, so I can't get it as target directly. listener isn't a target or anything. Target is the object to which the event is dispatched. But since the listener doesn't care about event.target, the change should be fine. Events do propagate from document to window, and from window to a WindowRoot object and from there to the TabChildGlobal, so https://searchfox.org/mozilla-central/source/toolkit/content/browser-content.js#1123-1124 works fine.

Comment on attachment 8930347 [details] [diff] [review] Bug 1416519 - use async event dispatcher. [Security approval request comment] > How easily could an exploit be constructed based on the patch? Not easy, it just changes the way we dispatch event. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. > Which older supported branches are affected by this flaw? 56, 57, 58 > If not all supported branches, which bug introduced the flaw? bug 1274919 > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I can also uplift it to 58, and it should be no risky. > How likely is this patch to cause regressions; how much testing does it need? It has low possibility to cause regressions, since we just avoid to dispatch event after CC. We've had a browser test for that.

sec-approval+ for trunk. Please nominate a patch for the Beta branch as well.

Comment on attachment 8930347 [details] [diff] [review] Bug 1416519 - use async event dispatcher. Approval Request Comment [Feature/Bug causing the regression]: bug 1274919 [User impact if declined]: Unknown things may happen if that assertion fires [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: No [Is the change risky?]: No [Why is the change risky/not risky?]: just change the way which we dispatch event from sync to async [String changes made/needed]: No

Comment on attachment 8930347 [details] [diff] [review] Bug 1416519 - use async event dispatcher. Fix a sec-high. Beta58+.